Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix model argument issue (#1198) #1205

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Fix model argument issue (#1198) #1205

merged 3 commits into from
Dec 11, 2023

Conversation

ngocbh
Copy link
Contributor

@ngocbh ngocbh commented Dec 1, 2023

A quick fix to issue #1198 that makes sure one model is used throughout inject_adapter function in BaseTuner.

@HuggingFaceDocBuilderDev
Copy link

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

younesbelkada
younesbelkada approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! What do you think @BenjaminBossan ?

BenjaminBossan
BenjaminBossan requested changes Dec 4, 2023
View reviewed changes
Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Ngoc, for working on this fix.

Right now, in _mark_only_adaption_prompts_as_trainable, you allow model to be None and then do model = model or self.model. I would prefer to make model a required argument and remove the mentioned line. This way, it's not possible to accidentally forget to pass model and get unexpected results.

If this change is made, _mark_only_adaption_prompts_as_trainable could even be a standalone function, but that's not really a big concern.

Furthermore, there is one place where this method still needs to be adjusted, which should also make the CI pass:

peft/src/peft/tuners/mixed/model.py

Line 132 in 5bad88b

def _mark_only_adapters_as_trainable(self) -> None:

Apart from that, this change looks good.

@ngocbh
Copy link
Contributor Author

ngocbh commented Dec 6, 2023

The reason I want to _mark_only_adapters_as_trainable can also work without passing the model argument is the BaseTuner is already a model. So if we want to use this function outside the class we might have to call model._mark_only_adapters_as_trainable(model.model). I think this is the main use case of this function outside the class. But calling like this seems a little bit weird to me. That's why I let this function work without passing the model argument. What do you think?

If you still prefer to make model a required argument, I will change it back.

@BenjaminBossan
Copy link
Member

I think this is the main use case of this function outside the class.

I'm not quite sure I understand this. Do you mean that PEFT users may want to call model._mark_only_adapters_as_trainable? If that is so, I think we should add an explicit method/function for that, as _mark_only_adapters_as_trainable is supposed to be "private" and thus not called directly in user code.

@ngocbh
Copy link
Contributor Author

ngocbh commented Dec 9, 2023

Sorry for the delay, I fixed the issue according to your suggestions.

BenjaminBossan
BenjaminBossan approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks.

@BenjaminBossan BenjaminBossan merged commit 5c13ea3 into huggingface:main Dec 11, 2023
14 checks passed
@BenjaminBossan BenjaminBossan mentioned this pull request Dec 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminBossan BenjaminBossan BenjaminBossan approved these changes

@younesbelkada younesbelkada younesbelkada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ngocbh @HuggingFaceDocBuilderDev @BenjaminBossan @younesbelkada